Conversation
jasonish
requested review from
catenacyber
and removed request for
victorjulien
| DetectAppLayerInspectEngineRegister2("mqtt.connack.session_present", ALPROTO_MQTT, | ||
| SIG_FLAG_TOSERVER, 1, DetectEngineInspectGenericList, NULL); | ||
| SIG_FLAG_TOCLIENT, 1, DetectEngineInspectGenericList, NULL); |
There was a problem hiding this comment.
@catenacyber Here in the C version of registering this keyword we use a progress of 1. However, over in master when going through Rust its always a progress of 0. Should these be 0 over here? What was the reason for always using a progress of 0 instead of carrying over the values from C.
There was a problem hiding this comment.
I think there was no reason.
Does that change behavior ?
See cca59cd for some similar stuff...
You can try to have a MQTT transaction with a request and no answer yet (so tx progress is 1 to server, but 0 to client) and a rule to_client that gets evaluated and dropped before getting the data...
Does that make sense ?
There was a problem hiding this comment.
I don't know mqtt enough right now. But looks to be an oversight in the Rust registration of keywords?
There was a problem hiding this comment.
I guess a review of all progresses could be beneficial (some inconsistent progress seen in TLS for multi-buffer IIRC)
| DetectAppLayerMpmRegister2(BUFFER_NAME, SIG_FLAG_TOCLIENT, 2, PrefilterGenericMpmRegister, | ||
| GetData, ALPROTO_MQTT, 1); |
There was a problem hiding this comment.
Same here, we've changed the progress from 1 to 0 with the migration to Rust for keyword registration.
jasonish
force-pushed
the
7.0.x-7323-mqtt/v1
branch
from
fa7e64a to
5032a60
Compare
Backport of commit 5d82521. Ticket: 7323
jasonish
force-pushed
the
7.0.x-7323-mqtt/v1
branch
from
5032a60 to
d474393
Compare
|
Tests not backported, I think they require other changes, or perhaps side affects of the Rust migration. For example, the rule in this test: https://github.com/OISF/suricata-verify/blob/master/tests/mqtt-connect-rules-2/test.rules#L1 One cannot actually match on a non-zero reason code in 7.0.x it seems. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main-7.0.x #12155 +/- ##
==============================================
+ Coverage 83.13% 83.14% +0.01%
==============================================
Files 922 922
Lines 260821 260845 +24
==============================================
+ Hits 216826 216877 +51
+ Misses 43995 43968 -27
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 23562 |
Sounds like a bug... |
Just an observation so far. I don't know why. |
|
Within the scope of the backport do things look OK though? Any other changes in master ring a bell as to why this simple rule doesn't work in 7? |
|
Fixed in #12157 ;-) |
Just found this removal of an early return in 0a1062f that fixes the reason code issue. |
4 participants
Backport for https://redmine.openinfosecfoundation.org/issues/7324.